feat: reusable RPG scaffolding tier - Ruleset.Rpg + Ruleset.Basic (PLAN-0008)#18
Conversation
…AN-0008) Implements ADR-0008: extracts CombatantBehavior/CombatResolver/CombatManager/ AttackCommand/FleeCommand out of the Classic sample into a new SharpMud.Ruleset.Rpg package, decoupling its stats-behavior/respawn touches behind a new ICombatOutcomeHandler seam, adding a DI-registered IDiceRoller over IRandomSource, and fixing a pre-existing bug where a respawned character's CombatantBehavior.CurrentHitPoints was never reset. Adds a new minimal SharpMud.Ruleset.Basic package built on Ruleset.Rpg for a true "dotnet add package, few lines, run a basic game" quick-start. Classic is rebuilt on Ruleset.Rpg instead of owning combat scaffolding directly. Also: dependabot now ignores the conditional/range-pinned packages it can't reliably update, Directory.Packages.props centralizes the lockstep net10.0/net11.0 Microsoft.* version ranges into one property per TFM, and the docsite gains Rulesets/Customizing pages. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87fe97c689
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fixes two real death-penalty bugs found by review: CombatManager reset CombatantBehavior.CurrentHitPoints to full before the outcome handler ran, but neither ClassicCombatOutcomeHandler nor BasicCombatOutcomeHandler actually halved it back down, so the documented "respawn at half HP" penalty was silently a no-op for both rulesets. Also fixes a player-facing message-order regression (defeat message now precedes the outcome handler's own messages again, matching pre-extraction behavior), adds a guard so AttackCommand fails cleanly instead of crashing at tick time when the actor has no CombatantBehavior, adds fail-fast validation to BasicRulesetOptions, and adds XML doc comments across the new public surface in both packages. Also: docs/getting-started.md's package install commands now include --prerelease (alpha packages, none published as stable yet), and several docs/*.md sections left stale by the extraction (combat.md's code sketch, engine-vs-ruleset.md's ruleset-behaviors listing, a small ADR-0008 typo) are brought current. ADR-0008's Open Items are updated to record the decisions actually made during implementation (meta-package exclusion, the ICombatOutcomeHandler mechanism, and the forwarding-callback command composition), rather than left as still-open questions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Fixes a real, pre-existing bug FleeCommand carried over from before this PR's extraction: it moved the actor directly instead of publishing the same UseExitEvent request MoveCommand does, so a locked exit blocked a normal move but not a flee through the exact same exit. FleeCommand now checks the lock (or any future exit veto) before ending the encounter and moving. Also: adds the test coverage the first review-feedback fix was missing (CombatantBehavior.CurrentHitPoints assertions in Classic's outcome-handler test, plus a new BasicCombatOutcomeHandlerTests covering the same death penalty for Basic - neither existed before, so the HP-halving fix itself had no regression coverage where it actually mattered); narrows CombatantBehaviorConfiguration/BasicStatsBehaviorConfiguration to internal (never referenced by name outside their own assembly, discovered only via EF's ApplyConfigurationsFromAssembly reflection scan - confirmed still works internal via the persistence round-trip tests); and brings docs/combat.md, docs/architecture.md, docs/plans/0008-ruleset-scaffolding-tier.md, and docsite/docs/rulesets.md up to date with the HP-halving fix and the FleeCommand fix from this round. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c3f0c7407
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two more real bugs, both in shared combat scaffolding: - CombatManager tracked encounters keyed by attacker only, so a second player could start an independent encounter against a defender someone else was already fighting - both encounters would then resolve, remove, and award victory independently for the same kill (duplicate XP). Added ICombatManager.IsDefenderEngaged, checked by AttackCommand before starting a new encounter. - FleeCommand ignored Remove's return value and unconditionally added the actor to the destination room afterward. Remove can itself be vetoed (publishes a cancellable RemoveChildEvent) - proceeding anyway could add the actor to the new room without ever having left the old one, and ended the encounter regardless of whether the move actually happened. Now mirrors MoveCommand's check-then-proceed order. Also: Dockerfile was missing a COPY for the new SharpMud.Ruleset.Rpg.csproj in the early project-file layer, so `docker build` failed restoring Classic's csproj before src/ was copied wholesale - verified fixed with a real `docker build`, not just by inspection. BasicRulesetOptions' class summary is now a real XML doc comment (was a plain // comment, so it never made it into generated IntelliSense/NuGet docs). The docsite's ICombatOutcomeHandler sample now uses an explicit constructor instead of a primary constructor, matching the coding standard consumers are meant to copy. A couple of remaining stale docs mentions (ICombatant) cleaned up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa056f766d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ee rollback)
CombatManager's encounter tracking assumed single-threaded access ("world
mutation happens on the single game-loop thread"), but that's only true for
tick-vs-tick ordering - AttackCommand/FleeCommand run inside whichever
session's independently-scheduled task happens to be executing a command
(TelnetTransportBackgroundService runs one task per connection), so
_encounters is genuinely accessed from multiple threads. The previous
round's IsDefenderEngaged-then-StartEncounter guard was a real TOCTOU race:
two players targeting the same NPC at nearly the same time could both
observe "not engaged" before either inserted, recreating the duplicate-XP
bug. Replaced StartEncounter with an atomic TryStartEncounter (check +
insert under one System.Threading.Lock critical section) and made every
other _encounters access go through the same lock. Added a real concurrent
test (32 attackers racing the same defender via Task.WhenAll/Task.Run,
asserting exactly one wins) alongside the sequential case, since the
sequential case alone wouldn't have caught the original race.
Also: FleeCommand checked Remove's cancelable result but not
destination.Add's - a vetoed Add (e.g. a future "room is full" behavior)
would leave the actor already removed from the encounter and their old
room, with success already announced, but never actually added anywhere.
Now rolls back into the original room and reports failure instead.
docs/combat.md's ICombatManager snippet/sequence updated for
IsDefenderEngaged, TryStartEncounter, the "someone else is already
fighting" message, and switched off the stale primary-constructor sketch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ncipollina
left a comment
There was a problem hiding this comment.
🎉 Full review pass complete (coding-standards, design-decisions/ADR-0008, testing, persistence, docs, security) on top of the four prior review rounds already baked into this branch. Build clean, 148/148 tests pass. No 🐛 blocking findings - this would be an Approve, but GitHub won't let the PR author self-approve. One CombatManager.OnTickAsync (flee-then-immediately-reattack in the same tick window) - not blocking, flagged as a fast-follow. Everything else - the ICombatOutcomeHandler seam, TryStartEncounter's atomic check-and-insert, FleeCommand's Remove/Add rollback, EF mapping move, docs/ADR/plan bookkeeping - checks out clean. Ready to merge from my end.
The per-tick loop fetched an encounter under _lock, then resolved a round against that captured object outside the lock (necessarily, since holding a lock across an await isn't possible). A concurrent FleeCommand (ending this attacker's encounter) immediately followed by AttackCommand (starting a new one against a different defender) could land in that window - the tick would then resolve a round against the stale, already-replaced encounter, and could wrongly EndEncounter the new one afterward based on that stale round's outcome. Added a second lock-guarded re-check immediately before resolving each round: skip this tick for that id unless _encounters[thingId] is still reference-equal to what was fetched. Narrows the actual race window to nothing meaningful (no yield point between the check and the synchronous resolve call that follows it). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ncipollina
left a comment
There was a problem hiding this comment.
🎉 Final review: the _lock right before ResolveRound, no await in between - closes the window to nothing meaningful). Build clean, 30/30 Rpg tests + full suite pass. No 🐛 blocking findings remain. This would be an Approve, but GitHub blocks self-approval on your own PR - ready to merge from my end.
📋 Summary
Implements ADR-0008/PLAN-0008: extracts the reusable combat/encounter scaffolding out of
samples/SharpMud.Samples.Classicinto a newSharpMud.Ruleset.Rpgpackage, and adds a new minimalSharpMud.Ruleset.Basicpackage built on it for a true "dotnet add package, a few lines, run a basic game" quick-start.SharpMud.Samples.Classicis rebuilt onRuleset.Rpginstead of owning combat code directly.Two small unrelated housekeeping items are bundled in per request: dependabot now ignores packages it can't reliably version-range-update, and
Directory.Packages.propscentralizes the lockstepnet10.0/net11.0Microsoft.*version ranges into one property per TFM.📝 Changes
SharpMud.Ruleset.Rpg(new package)CombatantBehavior,ICombatResolver/CombatResolver,ICombatManager/CombatManager,AttackCommand/FleeCommandin from Classic.CombatManager's directStatsBehavior/hard-coded hub-room touches behind a newICombatOutcomeHandlerinterface (OnVictoryAsync/OnDefeatAsync) — each ruleset implements its own XP-award/death-penalty/respawn logic;CombatManageritself has zero reference to any concrete ruleset's types.CombatantBehavior.CurrentHitPointswas previously never reset (only the ruleset's own stats behavior was), so the next hit could instantly re-trigger "defeated" regardless of the roll.CombatManagernow resets it unconditionally on respawn.IDiceRoller/DiceRoller("N dice of M sides plus a modifier") over the existingIRandomSource, DI-registered, not a static singleton.RpgBehaviorMappingContributorsoCombatantBehavior's EF Core mapping moves with it (previously only Classic's assembly was scanned).AddSharpMudRpgRuleset<TCombatOutcomeHandler>(...)wires all of the above, and forwards an optional consumer command-registration callback internally sokill/attack/fleeand a consumer's own commands don't clobber each other viaHosting's single-registrationICommandRegistry.SharpMud.Ruleset.Basic(new package)BasicStatsBehavior(Level/Experience, no Race/Class),BasicWorldBuilder(two rooms + one fightable NPC),BasicPlayerFactory,BasicCombatOutcomeHandler.AddSharpMudBasicRuleset(...)with an options callback for starting HP/AC/damage.SharpMud.Samples.ClassicRuleset.Rpgand a newClassicCombatOutcomeHandler.Program.cs's manual combat wiring collapses to oneAddSharpMudRpgRuleset<ClassicCombatOutcomeHandler>()call.Tests
SharpMud.Ruleset.Rpg.Tests/SharpMud.Ruleset.Basic.Testsprojects (130 tests total across the solution).AttackCommand/FleeCommand/DiceRoller(none existed before).ICommandRegistry.CombatantBehavior's EF mapping survives the assembly move.Docs
docs/:architecture.md,engine-vs-ruleset.md,combat.md,character.mdupdated; newdocs/getting-started.md; ADR-0008 flipped toAccepted, plan toDone.docsite/: new Rulesets page (three-tier architecture, EF Core analogy,Ruleset.Basicquick-start, "build your own ruleset onRuleset.Rpg" walkthrough) and new Customizing sharp-mud page (Thing/Behavior model, "where does X belong" guidance, extension-point table) — both linked from nav and the homepage.Housekeeping (unrelated, bundled per request)
.github/dependabot.yml: ignore the 7 packages pinned as conditional version ranges inDirectory.Packages.props— Dependabot can't reliably update those.Directory.Packages.props: centralized the 6 lockstepMicrosoft.EntityFrameworkCore.*/Microsoft.Extensions.*packages behind oneMicrosoftPackagesVersionNet10/Net11property pair per TFM instead of repeating the range 6 times. Kept theConditionon theItemGrouprather than the property value — conditioning the property alone silently broke resolution for one project during testing (NuGet's TFM-agnostic static-graph restore pass evaluates the property before$(TargetFramework)is set).🧪 Validation
dotnet build/dotnet testclean on bothnet10.0andnet11.0, 130/130 tests passing, no NuGet restore warnings.samples/SharpMud.Samples.Classicend-to-end over the CLI transport —kill/attack/fleeresolve with real hit/miss/damage/XP messages. Built a throwawayProgram.csagainst justSharpMud.Ruleset.Basic+Engine/Hosting/Persistence.Sqlite/Adapters.Cliand confirmed a fresh login → walk →kill→ XP award → SQLite persistence across shutdown, end to end.💬 Notes for Reviewers
ICombatOutcomeHandlershape and the forwarding-callback command-composition approach were both left as open/plan-level decisions in ADR-0008 — happy to discuss alternatives if either doesn't sit right.docs/getting-started.md(contributor-facing) anddocsite/docs/rulesets.md(public-facing) currently overlap somewhat in content/purpose — flagged as a possible follow-up to reconcile, not addressed in this PR.